Skip to content

feat: add support for self hosted gateway #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 6, 2023

Conversation

cyclimse
Copy link
Contributor

Summary

What's changed?

The API framework can now manage routes for the self-hosted gateway project.

Why do we need this?

To provide the functionality to @app.get(...) decorators.

How have you tested it?

There are unit tests that mock the gateway with responses.
As well as an integration test.

Checklist

  • I have reviewed this myself
  • There is a unit test covering every change in this PR
  • I have updated the relevant documentation

Details

Currently, the integration test will fail because of tiny issues with the gateway. I've opened some PRs to address them.

@cyclimse cyclimse self-assigned this Mar 27, 2023
@cyclimse cyclimse marked this pull request as ready for review March 27, 2023 08:14
@cyclimse cyclimse requested review from Shillaker and redanrd March 27, 2023 10:07
)

import time

# TODO?: delete this. It's a kubernetes side-effect.
time.sleep(30)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is flaky, unfortunately :( Sometimes the updated function is marked as ready but it's older version will still respond to requests for up to a ~~minute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very random though which is why the CI passed when we merged those tests in #55.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do the same trick of retrying every 5 seconds for 50 seconds and only passing if it's the updated function response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is looking great. My only major comment is around the integration test setup.

I'll ping you on Slack to discuss.


time.sleep(60)
# TODO?: delete this
time.sleep(30)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With sleeps like this it's cleaner to add a loop that retries up to some limit. Perhaps we can add a utility function to trigger a function, then sleep for 5 seconds and retry, up to 10 retries?

)

import time

# TODO?: delete this. It's a kubernetes side-effect.
time.sleep(30)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do the same trick of retrying every 5 seconds for 50 seconds and only passing if it's the updated function response?

return key


def _deploy_gateway(project_id: str, client: Client) -> sdk.Container:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried about duplicating the gateway logic in this test. Instead, it might be cleaner and safer to have a setup step in the integration tests which did the following:

  1. Check out a tag of the API gateway
  2. Run the relevant make commands to set up the gateway
  3. Run a (new?) make command to get any valid auth key
  4. Run another (new?) make command to get the gateway URL
  5. Export this auth key and gateway URLs as environment variables to be used in this test

As I've noted, I'm not sure if the make interface in the gateway supports this, but it would be nice if it did. If it works, this could then become a reusable Github Action to set up and use your own gateway.

@cyclimse cyclimse force-pushed the feat/add-support-for-self-hosted-gateway branch from a07a3aa to 3d935ea Compare April 4, 2023 08:42
@cyclimse cyclimse requested a review from Shillaker April 4, 2023 17:35
Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM

@cyclimse cyclimse merged commit 161d3b4 into main Apr 6, 2023
@cyclimse cyclimse deleted the feat/add-support-for-self-hosted-gateway branch April 6, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants